Skip to content

Update cargo-fuzz setup; split fuzz_value_sanitize so it runs #6372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 22, 2025
Merged

Update cargo-fuzz setup; split fuzz_value_sanitize so it runs #6372

merged 3 commits into from
Aug 22, 2025

Conversation

moodmosaic
Copy link
Member

This makes clarity/fuzz/ build and actually run on nightly. It also moves fuzz_value_sanitize into its own target so cargo-fuzz detects and runs it. Diff kept minimal.

Notes:

  • This fixes compiler errors to keep Expand input coverage with fuzzing #6355 moving.
  • I haven’t reviewed what these existing fuzz targets actually test. They’ve been in the repo for some time. I kept the diff minimal to get them compiling and running.

How to:

  • List: cargo +nightly fuzz list
  • Build all: cargo +nightly fuzz build
  • Build one: cargo +nightly fuzz build fuzz_sanitize
  • Build one: cargo +nightly fuzz build fuzz_value_sanitize
  • Run: cargo +nightly fuzz run fuzz_sanitize
  • Run: cargo +nightly fuzz run fuzz_value_sanitize

@moodmosaic
Update cargo-fuzz setup; split fuzz_value_sanitize so it runs
2c1b0d0 
- Fix compiler errors in `clarity/fuzz/` and refresh dependencies.
- Move `fuzz_value_sanitize` into its own fuzz target so `cargo-fuzz` runs it.

Notes:
- Now builds and runs on `+nightly`.
- I have not fully reviewed semantic validity; kept the diff minimal.
- Prelude for stacks-network#6355 (Expand input coverage with fuzzing).

How to:
- List: `cargo +nightly fuzz list`
- Build: `cargo +nightly fuzz build [target]`
- Run: `cargo +nightly fuzz run fuzz_value_sanitize`
@moodmosaic moodmosaic requested review from a team as code owners August 12, 2025 13:54
@moodmosaic
Copy link
Member Author

/cc @BowTiedRadone

@moodmosaic moodmosaic requested a review from kantai August 12, 2025 14:23
@moodmosaic moodmosaic requested a review from a team as a code owner August 13, 2025 12:30
@moodmosaic moodmosaic requested a review from wileyj August 13, 2025 12:31
@moodmosaic moodmosaic force-pushed the fuzz-cov-6355/00-unbreak-cargo-fuzz branch from ad92993 to f8db633 Compare August 13, 2025 12:32
@aldur aldur requested a review from fdefelici August 13, 2025 15:22
fdefelici
fdefelici reviewed Aug 14, 2025
View reviewed changes
Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve added a bunch of comments about the workflows.

I’d also like to raise a broader structural idea: instead of keeping the fuzz folder nested inside the clarity crate, would it make sense to move it under the contrib/ folder?

We could, for example, introduce a dedicated fuzzing/ folder inside contrib/, which could later house multiple fuzzers as we expand coverage. The current clarity/fuzz would simply become one of them:

contrib/  
    fuzzing/  
        clarity-fuzzer/   (current clarity/fuzz)  
        other1-fuzzer/  
        other2-fuzzer/  
        ...

This would avoid nested crates inside clarity, make the structure more consistent, and give us room to organize future fuzzers without cluttering core crates.

If we agree this structure is beneficial, we could address it in this PR. The effort should be minimal, and since we’re already restoring the old fuzzer, could be the right time to make this change.

@moodmosaic
Copy link
Member Author

instead of keeping the fuzz folder nested inside the clarity crate, would it make sense to move it under the contrib/ folder?

Right now it makes more sense to keep the fuzz folder inside the clarity crate.

That way it matches how we handle example-based and property-based tests, and keeps fuzzing a first-class part of the folder/crate rather than something tucked away in contrib/.

Also, with this structure, cargo fuzz works naturally from the crate root without needing --manifest-path.

Moving it under contrib/ pays off when/if we had multiple independent fuzzers with shared arbitraries. Until then, local fuzz targets keep the workflow simple: "open the crate and see all its tests, including fuzz".

I think it’s a good idea to tackle reorganizing later, once we add more fuzzers, so we can keep #6355 moving.

@moodmosaic moodmosaic force-pushed the fuzz-cov-6355/00-unbreak-cargo-fuzz branch from f8db633 to 96c7ae8 Compare August 21, 2025 11:56
@moodmosaic moodmosaic requested a review from fdefelici August 21, 2025 11:56
fdefelici
fdefelici reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just adding a heads-up so we can make a common decision regarding cache management.

kantai
kantai reviewed Aug 21, 2025
View reviewed changes
kantai
kantai reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I just had a question about re-using the generator implementations across the two targets.

@moodmosaic moodmosaic requested review from kantai and fdefelici August 21, 2025 22:15
wileyj
wileyj approved these changes Aug 22, 2025
View reviewed changes
Copy link
Collaborator

@wileyj wileyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@moodmosaic moodmosaic added this pull request to the merge queue Aug 22, 2025
Merged via the queue into stacks-network:develop with commit 6a2f682 Aug 22, 2025
841 of 853 checks passed
@moodmosaic moodmosaic deleted the fuzz-cov-6355/00-unbreak-cargo-fuzz branch August 22, 2025 13:56
@codecov Codecov
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.22%. Comparing base (c21aea7) to head (ab5c9ce).
⚠️ Report is 158 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #6372      +/-   ##
===========================================
+ Coverage    75.81%   80.22%   +4.41%     
===========================================
  Files          552      555       +3     
  Lines       351807   351247     -560     
===========================================
+ Hits        266724   281791   +15067     
+ Misses       85083    69456   -15627

see 259 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c21aea7...ab5c9ce. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions GitHub Actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wileyj wileyj wileyj approved these changes

@fdefelici fdefelici fdefelici approved these changes

@kantai kantai Awaiting requested review from kantai

Assignees
No one assigned
Labels
locked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moodmosaic @kantai @wileyj @fdefelici